[BUGFIX canary] Meta refactor #11966

ef4 · 2015-08-03T10:31:11Z

This refactors the meta object as a step toward optimizing it better.

consolidates and encapsulates meta's data structures so we can more easily replace them
switches to explicit inheritance between meta objects over prototypical inheritance, so we can pick and choose how each member is actually inherited, and choose to do optimizations.
refactors meta.listeners in an attempt to make listener creation and deletion cheaper, especially in terms of allocations. This is still an early sketch and more effort is needed to see if it's actually a good idea.

stefanpenner · 2015-08-03T15:14:01Z

packages/ember-metal/lib/meta.js

+  Meta.prototype['writable' + capitalized] = function(create) {
+    let ret = this[key];
+    if (!ret) {
+      ret = this[key] = create(this.source);


I believe we can do this in a quicker way, as long as we are aware which are for instances and which are for classes.

basically, for an instance if we could do

ret = this[key] = new InheritedObjectSubclass();

for classes we would do something like:

function InheritedObjectSubclass() { } InheritedObjectSubclass.prototype = create(this.source);

we would still have 1 shape per InheritedObjectSubclass (which still will likely cause grief), but we could fast-path the instantiation for instances.

Interesting. Right now there is a single shape at every level, but you're saying it may be worth it to accept a shape-per-class in order to make instance creation faster, since presumably there are so many more instances than classes.

I guess we would need to try and measure the impact, I am uncertain. Deoptimizing some key method due to multiple-shapeness could be way worth than the instance instantiation cost.

but you're saying it may be worth it to accept a shape-per-class in order to make instance creation faster

You know, if we do the readable/writable thing correctly, the above likely doesn't matter. As we will now only lazily create the children. In all common cases, I hope that means that we just wont incur the cost of the children at all.

We may want to add logging to detect when this assumption is violated.

We can keep it up our sleeves, but after reading the full PR, I believe wont provide any value.

ef4 · 2015-08-04T18:43:41Z

I think we should merge this, so further work can be based off it.

I would also like to get perf feedback from apps on canary, to confirm that we are at least not making anything worse by accident.

rwjblue · 2015-08-04T18:57:39Z

Restarted sauce labs job.

rwjblue · 2015-08-04T19:08:45Z

I'm 👍 on landing this in canary.

stefanpenner · 2015-08-04T19:11:58Z

I would also like to get perf feedback from apps on canary, to confirm that we are at least not making anything worse by accident.

Let me get some time this afternoon to run this in some apps, just want to do our due diligence.

stefanpenner · 2015-08-04T20:53:35Z

packages/ember-metal/lib/empty_object.js

+  }
+});
+
+function EmptyObject() {}


slightly more concise version is possible

export default function EmptyObject() { } EmptyObject.prototype = null

6ms -> noise turns out there is no 0 cost abstraction in JS yet...

stefanpenner · 2015-08-04T22:09:18Z

something of interest, i have added some ghetto instrumentation to get a picture of how the new code is being utilized:

_createOwnMap: 3008
_geInherited: 25355
_getOrCreateInheritedMap: 17289
_getOrCreateInheritedMap.create: 110
_getOrCreateInheritedMap.fromParent: 5287
_getOrCreateOwnMap: 5269
inheritedCustomObject_readableChains: 2220
inheritedCustomObject_writableChains: 0
inheritedMapOfMaps_getAllDeps: 0
inheritedMapOfMaps_readableDeps: 6
inheritedMapOfMaps_writableDeps: 4404
inheritedMap_clearBindings: 0
inheritedMap_clearMixins: 0
inheritedMap_clearValues: 0
inheritedMap_clearWatching: 0
inheritedMap_peekBindings: 0
inheritedMap_peekMixins: 5405
inheritedMap_peekValues: 0
inheritedMap_peekWatching: 15367
inheritedMap_readAbleBindings: 2357
inheritedMap_readAbleMixins: 0
inheritedMap_readAbleValues: 0
inheritedMap_readAbleWatching: 0
inheritedMap_writableBindings: 0
inheritedMap_writableMixins: 188
inheritedMap_writableValues: 0
inheritedMap_writableWatching: 7410
ownCustomObject_readableChainWatchers: 7438
ownCustomObject_writableChainWatchers: 0
ownMap_reableCache: 28
ownMap_writable_Cache: 5269

the following is likely the worse offender in-terms of performance.

_getOrCreateInheritedMap.fromParent: 5287

it appears the most costly, are listeners to descriptors that likely never change (at least not in this example). In addition, these listeners could actually be inherited listeners, as the CP in question are defined on the classes, not the instances.

It seems like an interesting optimization to pursue at a future point in time.

In one example this removes nearly 8% of gets

speed up Meta instantiation

[BUGFIX canary] Meta refactor

stefanpenner · 2015-08-05T03:29:15Z

this is a great start in cleaning up our internals. Good job @ef4, this will be nice to work on-top of for our next opts.

sandstrom · 2015-08-16T21:14:19Z

packages/ember-metal/tests/meta_test.js

+  }
+});
+
+QUnit.test('meta is not enumerable', function () {


I was just reading through this PR out of curiosity. There may be a reason I don't understand, but this test seem to be repeated twice.

Ya, seems like a dupe. Mind submitting a PR to remove the duplicate?

ef4 added 30 commits August 2, 2015 15:12

DRY up meta()

a5dcce8

moving meta into its own module

8133291

make exports consistent

3044547

notes-to-self on each meta property's access patterns

ab25153

encapsulated meta.cache

807b963

generalize upgrade support

31c16f0

more progress on generic meta accessors

4bb9009

encapsulated meta.watching

d885f23

align semantics

542765b

lazy watchers

a4c9dd3

encapsulated meta.mixins

68895ec

encapsulate meta.bindings

aab8f9a

encapsulated meta.values

0b96990

typos

bbfb0e7

extra reusable bits

c40512a

encapsulate meta.listeners

bab0092

encapsulate meta.deps

9b51c9e

encapsulate meta.chainWatchers

1fd3eb5

encapsulate meta.chains

a3f86cc

break the meta prototype chain!

93743c1

better naming

d51a492

better naming for inheritedMap accessors

f216afc

better naming for inheritMapOfLists accessors

a4a4854

better naming for inheritedMapOfMaps accessors

2d70d56

better naming for ownCustomObject accessors

d748879

better naming for inheritedCustomObject accessors

dbc712c

commenting

8886eb7

spelling, how does it work?

45d73c1

starting some new meta tests

26008e0

point everything at the new meta module

c4b3bce

stefanpenner reviewed Aug 3, 2015
View reviewed changes

ef4 added 4 commits August 3, 2015 13:34

micro-optimize every Object.create(null) to our own EmptyObject

7b5fcc0

fix silly unnecessary defProp

c64b316

incorporating stef feedback

09218cf

use empty object

714f0b8

ef4 changed the title ~~[WIP] Meta refactor~~ [BUGFIX canary] Meta refactor Aug 4, 2015

stefanpenner reviewed Aug 4, 2015
View reviewed changes

speed up Meta instantiation

580a40a

6ms -> noise turns out there is no 0 cost abstraction in JS yet...

stefanpenner and others added 3 commits August 4, 2015 16:34

don’t event try the for loop if there are no actions

a64e211

remove get for an internal property that will never be a CP

b6b0739

In one example this removes nearly 8% of gets

Merge pull request #2 from stefanpenner/eds

584fc73

speed up Meta instantiation

stefanpenner added a commit that referenced this pull request Aug 5, 2015

Merge pull request #11966 from ef4/meta-sanity

41ece2b

[BUGFIX canary] Meta refactor

stefanpenner merged commit 41ece2b into emberjs:master Aug 5, 2015

sandstrom reviewed Aug 16, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX canary] Meta refactor #11966

[BUGFIX canary] Meta refactor #11966

ef4 commented Aug 3, 2015

stefanpenner Aug 3, 2015

ef4 Aug 3, 2015

stefanpenner Aug 3, 2015

ef4 commented Aug 4, 2015

rwjblue commented Aug 4, 2015

rwjblue commented Aug 4, 2015

stefanpenner commented Aug 4, 2015

stefanpenner Aug 4, 2015

stefanpenner commented Aug 4, 2015

stefanpenner commented Aug 5, 2015

sandstrom Aug 16, 2015

rwjblue Aug 16, 2015

[BUGFIX canary] Meta refactor #11966

[BUGFIX canary] Meta refactor #11966

Conversation

ef4 commented Aug 3, 2015

stefanpenner Aug 3, 2015

Choose a reason for hiding this comment

ef4 Aug 3, 2015

Choose a reason for hiding this comment

stefanpenner Aug 3, 2015

Choose a reason for hiding this comment

ef4 commented Aug 4, 2015

rwjblue commented Aug 4, 2015

rwjblue commented Aug 4, 2015

stefanpenner commented Aug 4, 2015

stefanpenner Aug 4, 2015

Choose a reason for hiding this comment

stefanpenner commented Aug 4, 2015

stefanpenner commented Aug 5, 2015

sandstrom Aug 16, 2015

Choose a reason for hiding this comment

rwjblue Aug 16, 2015

Choose a reason for hiding this comment